Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

docs: add explanation for ACK and NACK semantics #9309

Merged
merged 5 commits into from
Dec 16, 2019
Merged

docs: add explanation for ACK and NACK semantics #9309

merged 5 commits into from
Dec 16, 2019

Conversation

sanjaypujare
Copy link
Contributor

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: More precise explanation for ACK and NACK for implementers' benefit
Risk Level: Low (doc only change)
Testing: doc only testing (using github rst rendering)
Docs Changes: More precise explanation for ACK and NACK for implementers' benefit
Release Notes: N/A
[Optional Fixes #Issue]
[Optional Deprecated:]

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9309 was opened by sanjaypujare.

see: more, trace.

@sanjaypujare
Copy link
Contributor Author

@markdroth PR as discussed - pls review.

markdroth
markdroth reviewed Dec 11, 2019
View reviewed changes
Copy link
Contributor

@markdroth markdroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fine to me, modulo the one comment below. @htuch should review as well.

api/xds_protocol.rst Outdated Show resolved Hide resolved
htuch
htuch suggested changes Dec 11, 2019
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarifications!
/wait

api/xds_protocol.rst Outdated Show resolved Hide resolved
api/xds_protocol.rst Outdated Show resolved Hide resolved
@dio
Copy link
Member

dio commented Dec 12, 2019

@sanjaypujare seems like you need to fix DCO. https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#fixing-dco.

@sanjaypujare
Copy link
Contributor Author

@sanjaypujare seems like you need to fix DCO. https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#fixing-dco.

Thanks! Done.

@sanjaypujare
Copy link
Contributor Author

@htuch @ramaraochavali friendly ping to review and merge as appropriate.

htuch
htuch reviewed Dec 13, 2019
View reviewed changes
api/xds_protocol.rst Outdated
management server a shared notion of the currently applied configuration,
as well as a mechanism to ACK/NACK configuration updates.

ACK:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use proper RST syntax here? I suggest previewing the docs. @markdroth can probably help spin you up IRL on how this works. Thanks

htuch
htuch suggested changes Dec 13, 2019
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Content LGTM, just a formatting ask remaining at my end.
/wait

@sanjaypujare
Copy link
Contributor Author

Content LGTM, just a formatting ask remaining at my end.
/wait

Fixed the format. Please take a look.

htuch
htuch reviewed Dec 16, 2019
View reviewed changes
api/xds_protocol.rst Outdated


.. figure:: diagrams/later-ack.svg
:alt: ACK after NACK

ACK and NACK semantics can be summarized as follows:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The content below this isn't specific to NACK anymore, so needs a new header (or reorging). Also, DCO needs fixing before we can merge. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@htuch fixed both. Thanks

@htuch htuch added the waiting label Dec 16, 2019
@sanjaypujare
docs: add explanation for ACK and NACK semantics
a971777 
Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>
@sanjaypujare
docs: address review comments
d1a7a08 
Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>
@sanjaypujare
address more comments
7a0008d 
Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>
@sanjaypujare
docs: use RST format for subsection header
30396a4 
Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>
@sanjaypujare
docs: create sections for ACK/NACK and versioning
4d9658d 
Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>
htuch
htuch approved these changes Dec 16, 2019
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@htuch htuch merged commit 89aec85 into envoyproxy:master Dec 16, 2019
prakhag1 pushed a commit to prakhag1/envoy that referenced this pull request Jan 3, 2020
@sanjaypujare @prakhag1
docs: add explanation for ACK and NACK semantics (envoyproxy#9309)
d18d7dd 
More precise explanation for ACK and NACK for implementers' benefit

Risk Level: Low (doc only change)
Testing: doc only testing (using github rst rendering)

Signed-off-by: Sanjay Pujare <sanjaypujare@users.noreply.github.com>
Signed-off-by: Prakhar <prakhar_au@yahoo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@markdroth markdroth markdroth left review comments

@ramaraochavali ramaraochavali ramaraochavali left review comments

@htuch htuch htuch approved these changes

Assignees

@htuch htuch

@ramaraochavali ramaraochavali

@markdroth markdroth

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sanjaypujare @dio @htuch @ramaraochavali @markdroth